-
Notifications
You must be signed in to change notification settings - Fork 762
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[entropy_src/dv] Separate external health test (XHT) verification from V2 tests #17638
[entropy_src/dv] Separate external health test (XHT) verification from V2 tests #17638
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks fantastic, thanks @andreaskurth for having a look at this!
@@ -1107,6 +1108,21 @@ class entropy_src_scoreboard extends cip_base_scoreboard#( | |||
end | |||
endtask | |||
|
|||
task process_xht(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great idea to add such a check!
Nit: should this task rather be named check_xht_response()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, at some point the scoreboard will have to take XHT transactions into account for its modeling and checking, as it already does for TL accesses, for example. By convention, these tasks seem to start with process_
, e.g., process_tl_access
is defined for our scoreboards derived from cip_base_scoreboard
. So I think process_xht
is appropriate.
Note that in a proper UVM design, the scoreboard would not sample I/Os directly but use TLM analysis FIFOs (again using the TL example, the scoreboard calls process_tl_fifo
-> process_tl_a_item
-> process_tl_access
). I decided against this design for now for two main reasons:
- It's overly complicated for the check we need right now.
- As we want to ensure the DUT sees a fixed value in every cycle, directly sampling I/O is less error-prone than the indirection through a monitor and analysis FIFOs.
When implementing XHT tests, the checks in this PR can be complemented by or moved to TLM-based checks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
@@ -73,6 +73,13 @@ | |||
uvm_test_seq: entropy_src_rng_vseq | |||
} | |||
|
|||
{ | |||
name: entropy_src_rng_with_xht_rsps |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NB: This test has the default number of reseeds (50), whereas entropy_src_rng
still has 300. The number of reseeds can be increased later depending on XHT coverage (#16276)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good.
I've tested this PR locally with 300 seeds of |
When this knob is enabled, the default XHT response is always provided to `entropy_src`. Signed-off-by: Andreas Kurth <[email protected]>
This commit extends `entropy_src_scoreboard` to check that the default XHT response is indeed always applied if the corresponding knob is set. Signed-off-by: Andreas Kurth <[email protected]>
This commit changes the default value of the knob that tells tests to always provide the default XHT response to 1 (true). The rationale is that this is the same behavior as in the current top-level design, so we want to make sure that all existing tests work in an environment in which `entropy_src` is always given the default XHT response. The only test affected by this is `entropy_src_rng`. In order to not completely remove any test that provides time-varying XHT responses, this commit creates a new test, `entropy_src_rng_with_xht_rsps`, in which the `xht_only_default_rsp` knob is set to zero. This test is equivalent to the `entropy_src_rng` test before this commit. Signed-off-by: Andreas Kurth <[email protected]>
Signed-off-by: Andreas Kurth <[email protected]>
6f57caa
to
ec3e4b2
Compare
Rebased to include CI fixes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for looking at this issue. I've retriggered failed jobs on CI, let's wait for those to finish before merging.
As described in #17358, the current instantiation of
entropy_src
in the Earlgrey top-level design has the XHT response input tied off to a default value, but theentropy_src_rng
block-level test had time-varying, non-default XHT responses.This PR changes that in three commits as follows:
entropy_src_rng
with time-varying XHT responses to a newentropy_src_rng_with_xht_rsps
test and changeentropy_src_rng
to only drive default XHT responses.This resolves #17358.